Generated by 🚫 dangerJS against 5e4abf3

Hey @oliviertassinari,
I am new to this whole contributing thing,
Could you please point me in the right direction ?
I mean is using ButtonBase a good idea ?

@Tarun047 Hey. Yes, I think that using ButtonBase is the right path to get the ripple effect. I have labeled "needs revision" because the build is failing. @mbrookes has built this component int he first place, it would be great to have his insight on the matter.

@mbrookes Could you throw some light on this issue ?

You're heading in the right direction. The MD spec didn't have a ripple for the chip when this component was first created, but I'm glad to see that it was added since.

One thing to consider is under what circumstances a clicked chip should ripple. At the moment the Chip behaves differently depending on whether an onClick (or onDelete) function is provided. (This is a little different to other components, for better or worse...) You could do the same for the ripple, rather than having it always ripple when clicked.

At the moment the Chip behaves differently depending on whether an onClick (or onDelete) function is provided.

@mbrookes Interesting. For the multi-select case, we need the chip to be focusable and to have a delete button. Vuetify can be used to benchmark: https://vuetifyjs.com/en/components/chips#chips.

Would the following work?

• basic: no ripple, the chip is a static div
• onClick: focus + hover styles + ripple
• onDelete: focus + hover styles

For the multi-select case, we need the chip to be focusable and to have a delete button.

That's already supported. The ripple on click is incidental to that use case.

@mbrookes Agree, chip for multi-select is perfect right now, I don't want to have a regression :)

I made the changes, but tests are still failing due.
Can anyone help me with this ?

thanks for updating the tests @oliviertassinari

Should the clickable chip ripple when the delete icon is clicked?

Should the clickable chip ripple when the delete icon is clicked?

I would say no, but what's the use case for a clickable chip with a delete icon?

It's in the spec:

They don't show a specific example, but I would think something like an expandable chip that can also be deleted?

@mbrookes I have no solution to propose 🤔

Should the clickable chip ripple when the delete icon is clicked?

Giving more thought to this problem, I would be fine if we keep the ripple: we have customization demos where the delete icon is replaced with an end leading icon. I would propose to get developers feedback before changing the behavior.

we have customization demos

In the Chip playground? Perhaps a checkmark is not the best choice as an alternate delete icon?

In any case, I would suggest that if we can, we support the default use-case out-of-the-box, and wait for developer feedback on the need for an alternative. 🙂

I meant the checked icon in the very first demo:

Ok, so it's good to go, as is?

I meant the checked icon in the very first demo

Oops, was too busy looking for a customisation demo!

so it's good to go, as is?

I don't think the chip should ripple when the delete icon is clicked. However, it may not be a problem in practice if the deleted chip is immediately removed from the DOM.

In other words, it only appears "wrong" in our static demos, because the chip isn't removed when delete is clicked.

The caveat here is that in the Chip spec under Behaviour / Transformative, the example image appears to show the added chip having a grow transition. This could be taken to imply that a deleted chip should have the reverse. In that case, a flash of ripple might be undesirable.

On balance, I think it's okay to merge as is, and wait to see if it's considered a problem.

I think clicking/focusing the delete button inside the chip should not bubble up i.e. stop propagating the events. Then the whole chip should stop rippling. This also makes more sense since you probably don't want the chip click handler to be executed when delete is clicked.

I would be eager to collect more user feedback on this. From a product perspective, do we have enough context?

The stop propagation could be a valid solution. I guess we would need to stop mousedown, mouseup, touchstart, touchend, & onclick.

@Tarun047 Thank you!